Skip to content

feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #01, stacked on #3)#4

Merged
rjzondervan merged 4 commits into
feat/filter-chain-dispatchfrom
feat/asciihex-decode
May 28, 2026
Merged

feat(asciihex): ASCIIHexDecode encode + decode + chain wiring (PR #01, stacked on #3)#4
rjzondervan merged 4 commits into
feat/filter-chain-dispatchfrom
feat/asciihex-decode

Conversation

@rjzondervan

Copy link
Copy Markdown
Member

Summary

Implements PDF 1.7 §7.4.2 ASCIIHexDecode as the first of four filter PRs that attach to the chain dispatcher from #3. The dispatcher arm is a strict case extension — no structural change to the dispatcher itself.

Stacked on #3 feat/filter-chain-dispatch. This PR's base will auto-retarget to work/text-replacement once #3 merges.

What landed

Helper (on PDFObject, protected static) Purpose
ASCIIHexDecode($_stream, $params): string|false Strips PDF whitespace (§7.5.1), finds EOD >, pads odd trailing nibble with 0, accepts both cases, fail-safe on illegal chars
ASCIIHexEncode($_stream, $params): string Uppercase hex pairs + 80-col line wrap + EOD >; empty input emits just >

Both wired into apply_filter_chain_decode / apply_filter_chain_encode via case 'ASCIIHexDecode':.

Spec contract

Pinned by openspec/changes/feat-asciihex-decode/:

  • REQ-1 — ASCIIHexDecode SHALL decode per PDF 1.7 §7.4.2 (even-length, odd-length-pad, whitespace, lowercase, trailing-after-EOD)
  • REQ-2 — ASCIIHexEncode SHALL emit uppercase hex pairs with EOD (basic, empty, 80-col line wrap)
  • REQ-3 — Lossless round-trip on arbitrary binary input
  • REQ-4 — Illegal characters SHALL fail safely (p_error + raw input return; no exception)
  • REQ-5 — Chain dispatcher recognises /ASCIIHexDecode on both decode + encode (MODIFIED capability from feat(filter-chain): array-form /Filter dispatch in PDFObject (PR #05 — foundation) #3)

Test plan

  • php examples/poc-filter-roundtrip-asciihex.php — 5 REQs, 0 assertion failures
  • php examples/poc-replace-text.php — baseline still green (PoC FlateDecode path unchanged)
  • php examples/poc-filter-chain-roundtrip.php — dispatcher tests green after sentinel update
  • php -l clean on touched files
  • No new composer dependencies; PHP 7.4 compat preserved

Companion change to dispatcher's round-trip test

Dispatcher's "unknown filter" assertion (from #3) previously used /ASCIIHexDecode as the sentinel — now that ASCIIHexDecode is implemented, the sentinel needs to be a name that won't ever be a real filter. Updated to /SappTestUnknownFilter (5 LOC change in examples/poc-filter-chain-roundtrip.php). Future filter PRs (#2-#4) won't need to touch the sentinel.

Files changed (348 +, 23 −)

  • src/PDFObject.php — ASCIIHexDecode + ASCIIHexEncode helpers, dispatcher arms
  • examples/poc-filter-roundtrip-asciihex.php — new round-trip gate
  • examples/poc-filter-chain-roundtrip.php — sentinel update for forward-compat
  • docs/upstream-prs/01-asciihex-decode/{proposal,design,tasks}.md — implementation notes
  • openspec/changes/feat-asciihex-decode/tasks.md — 22/22 complete

Dependencies + ordering

Out of scope

…#1)

Implements PDF 1.7 §7.4.2 ASCIIHexDecode on top of the chain dispatcher
from PR #5. Both encode and decode go through the existing chain
plumbing — adding ASCIIHexDecode is a strict `case` extension to
apply_filter_chain_decode / apply_filter_chain_encode, no structural
change to the dispatcher itself.

New helpers on PDFObject (both `protected static`, PascalCase per
upstream's filter-name convention):

  - ASCIIHexDecode($_stream, $params): string|false
      Strips PDF whitespace (§7.5.1), finds EOD `>`, discards anything
      after, pads odd-length input with `0` per §7.4.2 ¶3, accepts
      both cases of hex digits. Illegal characters → p_error + return
      raw input (D2 fail-safe, matches upstream convention).

  - ASCIIHexEncode($_stream, $params): string
      strtoupper(bin2hex($_stream)) + chunk_split at 80 columns +
      EOD `>`. Empty input emits just `>`.

Dispatcher `case 'ASCIIHexDecode':` arms added to both encode + decode
chain helpers. The dispatcher's existing round-trip test (from PR #5)
updated to use `/SappTestUnknownFilter` as the unknown-filter sentinel
so it stays valid as subsequent filter PRs (#2-#4) land their own
case arms.

Contract pinned by openspec/changes/feat-asciihex-decode/
(proposal + design D1-D5 + spec REQ-1 through REQ-5 with MODIFIED
filter-chain-dispatch capability + tasks).

Verification:
  - examples/poc-filter-roundtrip-asciihex.php (new) — 5 REQs covered:
    decode scenarios (even-length, odd-length-pad, whitespace,
    lowercase, trailing-after-EOD), encode scenarios (basic, empty
    input, 80-col wrap), lossless round-trip on 1024 random bytes,
    illegal-char fail-safe, chain integration (single-filter and
    [/ASCIIHexDecode /FlateDecode] two-filter outer-envelope).
  - examples/poc-replace-text.php — baseline still green.
  - examples/poc-filter-chain-roundtrip.php — dispatcher tests pass
    after the unknown-filter sentinel update.

Implementation note for the eventual upstream PR (preserved in
docs/upstream-prs/01-asciihex-decode/): hex2bin + chunk_split-based
implementation keeps the code path under 40 LOC and dependency-free.
PHP 7.4 minimum preserved.

Closes openspec/changes/feat-asciihex-decode/tasks.md 22/22.
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php
Comment thread examples/poc-filter-roundtrip-asciihex.php
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php
Comment thread openspec/changes/feat-asciihex-decode/tasks.md Outdated
Comment thread docs/upstream-prs/01-asciihex-decode/design.md
Comment thread openspec/changes/feat-asciihex-decode/tasks.md Outdated
Comment thread docs/upstream-prs/01-asciihex-decode/tasks.md Outdated
Comment thread examples/poc-filter-roundtrip-asciihex.php
Comment thread examples/poc-filter-roundtrip-asciihex.php
Comment thread examples/poc-filter-roundtrip-asciihex.php
Comment thread src/PDFObject.php
Comment thread src/PDFObject.php Outdated
Comment thread docs/upstream-prs/01-asciihex-decode/design.md
Comment thread examples/poc-filter-roundtrip-asciihex.php
Comment thread examples/poc-filter-roundtrip-asciihex.php
Comment thread examples/poc-filter-roundtrip-asciihex.php Outdated

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict-mode review (REQUEST_CHANGES)

ASCIIHexDecode/Encode implementation is mostly spec-correct, but contains a critical contract bug: the decoder's documented string|false return is never false on failure (it returns the raw input), so the dispatcher's === false short-circuit is dead code and a corrupted chain silently passes garbage downstream; several other gaps in tests, docs, and edge cases.

Findings: 🔴 3 blockers · 🟡 6 concerns · 🟢 9 minors. All findings have been posted as inline comments above. Per Strict-mode rules, any 🟡 or 🔴 holds the verdict; address each thread before requesting re-review.

🔴 Blockers

(Inline comments include **Impact:** and **Suggested fix:** sections per Strict-mode body template.)

rjzondervan added a commit that referenced this pull request May 28, 2026
Adds two new commits on top of the existing PR #3 branch:

  1. The PR #1 fix cherry-picked: PoC's PDFDoc::replaceTextInDocument
     rename to snake_case + add_object write-back + escape-contract
     docblock + streams_scanned semantics + Phase-2 exhaustive sweep
     in the verify gate + namespace/named-arg fixes in
     poc-make-fixture.php. Same content as the corresponding PR #1
     fix; arrives here because PR #3 has a cherry-pick of the PoC
     and the PR #1 review's blockers apply to that copy too.

  2. This commit: PR #3-specific fixes from Wilco's first-pass
     strict review.

🔴 PR #3 Blocker
  - PHP 7.4 named-args in poc-make-fixture.php — addressed by
    commit (1) above (the cherry-picked PR #1 fix replaces named
    args with positional and pins gzcompress level to 6).

🟡 PR #3 Concerns
  - get_stream(false) on chain failure now returns `false` (matching
    upstream's pre-refactor `return p_error(...)` semantics) rather
    than the raw $this->_stream. Callers using the `if ($decoded ===
    false) continue;` idiom get the skip behaviour they did before
    the dispatcher refactor. spec.md REQ-5 scenario updated to match.
  - build_flate_params /Columns default fixed from 0 → 1 per
    PDF 1.7 §7.4.4.3 Table 8. Hidden today because Predictor=1
    short-circuits; would have bitten the moment a PNG-predicted
    stream landed without explicit /Columns.
  - REQ-2 (encode-decode round-trip lossless) spec scenario
    tightened to "Predictor absent or = 1" — round-trip with a
    non-trivial PNG predictor is acknowledged as out of scope (no
    encode-side predictor support); future `feat-flate-predictor-encode`
    closes the gap.
  - design.md D1 wording fixed from "We add two `private` helpers"
    to "We add two `protected static` helpers", matching what
    shipped. Visibility choice justified (test-suite subclasses can
    stub for chain-ordering assertions; matches FlateDecode).
  - design.md D6 rewritten to reflect what the gate actually
    exercises (5 contract scenarios including the new REQ-3 smoke
    test) instead of the original "synthetic two-filter PDF"
    promise. Two-filter ordering is locked at spec layer but
    proved inductively by upstream-PRs #1-#4 each pairing the new
    codec with /FlateDecode.
  - examples/poc-filter-chain-roundtrip.php gained a REQ-3 block
    smoke-testing /DecodeParms positional routing — single-filter
    chain with explicit `/DecodeParms [<</Predictor 1 /Columns 4>>]`
    that round-trips cleanly.
  - docs/upstream-prs/05-filter-chaining/{proposal,design,tasks}.md
    no longer carry the duplicated 18-line "Implementation note"
    block. Each file now links to
    `openspec/changes/feat-filter-chain-dispatch/design.md` as the
    canonical artefact — single source of truth.
  - docs/upstream-prs/README.md now explicitly explains the two
    numbering schemes: directory `01..08` is the upstream-submission
    order; fork PR numbers are the local landing order. Foundation
    (`05`) lands first locally so dependent codecs (`01..04`) can
    attach. Cross-references use the `upstream-PR #NN` form.
  - replaceTextInDocument camelCase fixed — via the PR #1 fix
    cherry-pick (commit 1 above) which renames to snake_case
    replace_text_in_document.

🟢 Minors deferred
  - normalise_filter_chain dead defensive branch — the branch
    handles a `null` input that may or may not be reachable
    depending on PDFObject construction path; leaving as-is.

Verification: both poc-replace-text.php (PR #1 gate) and
poc-filter-chain-roundtrip.php (this PR's gate, now with the REQ-3
smoke test block) exit 0 after these two commits.

NOTE on the stacked-PR / no-force-push approach: this PR's commit
diff still references the OLD scaffold content from PR #2 (the
pre-rewrite spec.md / proposal.md / etc.). PR #2's mechanical
pass (REQ-NNN numbering + GIVEN clauses + Status/Purpose stub
headers) is shipped on `chore/openspec-scaffold` and will arrive
here naturally when this branch is rebased onto an updated
`work/text-replacement` after PR #2 merges. The spec amendments in
this commit will then merge with PR #2's rewrites — small expected
conflicts in spec.md's REQ-5 + REQ-2 scenarios, resolvable inline.
# Conflicts:
#	examples/poc-filter-chain-roundtrip.php
Adds a merge commit bringing `feat/filter-chain-dispatch`'s fix
commits (PoC snake_case rename + chain dispatcher get_stream-returns-
false + Columns default fix + docs/upstream-prs deduplication) into
this branch, and a follow-up commit with PR #4 specific fixes.

🔴 PR #4 Blockers
  - ASCIIHexDecode failure-path return changed from `$_stream` (raw
    input) to `false`. Matches the chain dispatcher's `=== false`
    short-circuit contract and upstream `p_error`'s default return.
    Critical correctness fix: prior to this, a malformed outer-
    ASCIIHex layer in a `[/ASCIIHexDecode /FlateDecode]` chain
    silently fed the unmodified illegal bytes to the inner
    FlateDecode arm — silent data corruption.
  - PHPDoc on ASCIIHexDecode updated: `@return string|false` is now
    accurate (the failure paths return `false` per the previous
    fix). Description clarified to surface the chain-dispatcher
    contract requirement.
  - Negative chain test added in `poc-filter-roundtrip-asciihex.php`
    asserting the dispatcher short-circuits on outer-ASCIIHex
    failure: `[/ASCIIHexDecode /FlateDecode]` chain with malformed
    hex bytes MUST return `false` from `get_stream(false)`. This
    locks the failure-propagation contract that the previous bug
    silently bypassed.

🟡 PR #4 Concerns
  - 80-col line wrap edge case in `ASCIIHexEncode`: when the hex
    representation is a multiple of 80, the encoder previously
    appended `>` directly after the final chunk, producing an 81-
    char line. Now inserts a newline before `>` so no line exceeds
    80 chars. Locked by a new 40-byte (80-hex-char) test case.
  - `tasks.md` 1.2 corrected: shows the actual stacked-PR base
    (`feat/filter-chain-dispatch`) instead of `work/text-replacement`.
  - `tasks.md` 2.2 corrected: the failure-path returns `false` not
    raw input (matches the post-fix code).
  - `tasks.md` 4.1 corrected: case label is bare `'ASCIIHexDecode'`
    not `'/ASCIIHexDecode'` (the dispatcher strips the leading slash
    before the switch).
  - `docs/upstream-prs/01-asciihex-decode/{proposal,design,tasks}.md`
    no longer carry the duplicated 9-line implementation note.
    Each links to `openspec/changes/feat-asciihex-decode/design.md`
    as the canonical artefact (same pointer-only pattern as PR #3).

🟡 Concerns deferred to follow-ups
  - Numbering scheme reconciliation: covered by the
    `docs/upstream-prs/README.md` explainer in PR #3's fix commit
    (`upstream-PR #NN` vs fork PR # numbering), which arrives here
    via the merge.
  - Encode-dispatcher symmetry on infallible filters: ASCIIHexEncode
    documented as infallible; an explicit `=== false` symmetry would
    require a return-type contract change. Tabled.

🟢 Minors addressed in tests
  - Missing-EOD tolerance now has an assertion (`ASCIIHexDecode('48656C6C6F')
    === 'Hello'`).
  - Empty-input edge cases asserted: `'>'`, ` ' \n \t >'`.
  - Round-trip of empty input asserted: `decode(encode('')) === ''`.
  - Unnecessary `@` operator on `hex2bin` dropped (the alphabet
    validation above guarantees it can't legitimately fail).

🟢 Minors deferred
  - PHP case-insensitivity nit on `ASCIIHexDecode` collision —
    no action; PascalCase-on-filter-names is the established
    convention.
  - PHP 7.4 union return type in docs — the docs use PHPDoc-only
    notation already; the docs/upstream-prs note is reasonable.
  - ReflectionClass coupling — by design; documented.
  - `p_error` stdout assumption — acceptable; tests don't depend
    on the error channel routing.

Verification: all 3 gates exit 0
  - poc-replace-text.php (PR #1 gate)
  - poc-filter-chain-roundtrip.php (PR #3 gate)
  - poc-filter-roundtrip-asciihex.php (this PR's gate, with the
    new chain-failure-propagation test + 40-byte 80-col edge + the
    missing-EOD/empty-edge assertions)
… feat/asciihex-decode

# Conflicts:
#	openspec/changes/feat-asciihex-decode/specs/asciihex-decode-filter/spec.md
#	openspec/changes/feat-asciihex-decode/tasks.md

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — focused fix commit 70d9a3a cleanly addresses every 🔴 blocker and the substantive 🟡/🟢 findings from the first-pass review. Verified against src/PDFObject.php at head + the expanded examples/poc-filter-roundtrip-asciihex.php (chain-failure propagation, 40-byte 80-col boundary, missing-EOD tolerance, empty round-trip, EOD-first edges).

🔴 3/3 resolved

  • false-return on illegal charASCIIHexDecode now returns false on alphabet failure AND on defensive hex2bin failure; chain dispatcher's === false short-circuit now actually fires.
  • PHPDoc contract@return string|false accurate; doc body documents the chain-dispatcher rationale.
  • Negative chain test[/ASCIIHexDecode /FlateDecode] malformed-outer test asserts get_stream(false) === false.

🟡 5/6 resolved, 1 deferred with rationale

🟢 6/9 resolved, 3 deferred with rationale, 1 silent (non-blocking)

Resolved all 18 prior threads. No new findings. Cleared for merge once #3 lands.

@rjzondervan rjzondervan merged commit c43dc8b into feat/filter-chain-dispatch May 28, 2026
rjzondervan added a commit that referenced this pull request May 28, 2026
feat(runlength): RunLengthDecode encode + decode + chain wiring (PR #2, stacked on #4)
rjzondervan added a commit that referenced this pull request May 28, 2026
feat(lzw): LZWDecode + FlateDecode predictor refactor (PR #4, stacked on #6)
rjzondervan added a commit that referenced this pull request May 28, 2026
Blockers:
- Only the first match per TJ operator was processed. processTjArray
  returned on the first hit; outer caller advanced past the entire
  TJ. Multi-needle TJs and same-needle-twice-in-one-TJ
  under-counted replacements_per_needle. Rewrote with an outer
  while-loop that re-resolves fragment texts after each splice and
  continues until no remaining needle matches. New REQ-006 in
  spec.md anchors the contract with two-needle and same-needle-
  twice scenarios.
- Missing isset check on $stats['replacements_per_needle'][$needle]++
  in processTjArray. Lazy-init kept as a belt-and-braces guard
  (the entry point already pre-keys via array_fill_keys, but the
  guard survives future entry-point refactors).
- Odd-length hex silently dropped a char (@hex2bin($hex) ?: '').
  PDF 1.7 §7.3.4.3 mandates implicit trailing-zero padding. Now
  pads via `if (strlen($hex) & 1) $hex .= '0'`. New REQ-005
  scenario covers this.
- Non-spec leniency: numeric tokenizer accepted [-+\d.eE]. PDF 1.7
  §7.3.3 forbids exponent notation in Numeric Objects. Tightened
  to [-+.0-9] so a producer's `e` glyph after a hex CID can't be
  swallowed as part of a number. New REQ-005 scenario covers this.
- Phantom-ticked tasks 5.4 and 5.5 (no fixtures). Reverted to
  unchecked with explicit deferral notes.

Concerns:
- Dead $matchedOnce assignment removed via the rewrite.
- Comments not treated as whitespace inside TJ arrays. Now skipped
  per PDF 1.7 §7.2.4. REQ-005 scenario.
- tj_arrays_modified diagnostic key now pre-initialised at top of
  replace_text_in_document (contract no longer fragile).
- preg_match-per-byte for whitespace replaced with strpos against
  the 6-byte PDF whitespace set (~50× faster on large TJ arrays).
- Shape inheritance: placeholder now always emitted as literal
  regardless of first matched fragment's shape (matches D2
  documented contract).
- `@hex2bin` defensive `@` removed — the odd-length check above
  makes the warning unreachable; PHP errors for other malformed
  shapes now surface to callers explicitly.
- end() / reset() pointer-advancing avoided in the new render path
  (group-based emission uses numeric indexing).
- Triplicate Implementation Note blocks in docs/upstream-prs/07-tj-
  flattening/{proposal,design,tasks}.md replaced with canonical
  pointers (same pattern as PRs #1/#2/#3/#4/#5/#6).
- Tf set inside q/Q leaks documented as a known gap (q/Q stack
  not maintained); deferred to a follow-up.

Verification: all 7 gates green (poc-replace-text,
poc-tounicode-cmap, poc-tj-flattening, plus the 4 filter PoCs).

Also fixed in this commit: poc-tj-flattening.php was calling
$doc->replaceTextInDocument() (camelCase) which doesn't exist after
PR #1's snake_case rename. Updated to replace_text_in_document.
rjzondervan added a commit that referenced this pull request May 28, 2026
feat(asciihex): /ASCIIHexDecode filter (SAPP PR #1)
rjzondervan added a commit that referenced this pull request May 28, 2026
feat(lzw): /LZWDecode filter + applyPngPredictor refactor (SAPP PR #4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants